Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically create folder in project manager create and import #56420

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

nathanfranke
Copy link
Contributor

@nathanfranke nathanfranke commented Jan 2, 2022

Edit:

2023-09-07 - I am busy, but once I have time this will be my top priority PR.
2023-10-17 - I am working on this slowly 🙂
2023-10-18 - Rebased and happy, but should wait for 4.3 😃


Supersedes #49602
Fixes #60332
Fixes #74678
Resolves godotengine/godot-proposals#4749
Resolves godotengine/godot-proposals#2754
Partly r3solves godotengine/godot-proposals#7978

Behavior:

  • When create_dir is enabled, browse selects the containing/parent directory, project name is appended.
  • When create_dir is disabled, whether before or after browsing, browse selects the exact project folder.
  • Directory naming convention is configurable. kebab-case (default), snake_case, etc.

Potential improvements:

  • Behavior of capitalize() means test5 turns into test-5/. Fixed, now test5 --> test5/
  • Persist/remember if create_dir is pressed. Would we need an editor setting?
  • Maybe bug: horizontal scroll position of target path resets on change

Video:

2023-02-18.19-51-22_.mp4

Creation:
image

Import:
image

@Chaosus Chaosus added this to the 4.0 milestone Jan 2, 2022
@timothyqiu
Copy link
Member

timothyqiu commented Jan 2, 2022

When the project name is "Test" and "Create Folder" is on, project path is /path/to/dir/Test. If I then change the path to /path/to/dir/DoesNotExist and click "Create & Edit". It creates and uses the "DoesNotExist" folder for me.

I'm not sure if this is expected. If yes, would it be better to place the button somewhere else because it's now not "that" related to the project name as before.

Also, should "Create Folder" use a CheckBox instead of a CheckButton? Because the control does not have immediate effect (the folder is not immediately created).

To summarize:

  • What the "Create Folder" button does was clear: Create a folder with the project name at the current project path, and use that folder as the new project path.
  • The "Create Folder" toggle is confusing:
    • When toggled off: It strips the last component from project path no matter if it's the same as project name.
      1. Turn on
      2. Set the project path to /path/to/something/you/typed/later
      3. Turn off
      4. And it strips /later
    • When toggled on: It appends the project name to the project path, even when the current path does not exist.
      1. Set the project path to /path/that/does/not/exist
      2. Turn on
      3. The project path becomes /path/that/does/not/exist/ProjectName
      4. It still reports error because the parent directory does not exist (and won't be created).
    • When "Create & Edit" is pressed: create the project folder if missing when the toggle is on.

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Jan 3, 2022

When the project name is "Test" and "Create Folder" is on, project path is /path/to/dir/Test. If I then change the path to /path/to/dir/DoesNotExist and click "Create & Edit". It creates and uses the "DoesNotExist" folder for me.

This is the behavior I intended. Some users might want to make the directory name different from the project name (e.g. Card Game project name and card-game directory)

It be better to place the button somewhere else because it's now not "that" related to the project name as before.

I agree that moving the button somewhere else would make sense, but the only other place I can think of would be below the name/path fields, taking more vertical space.

Also, should "Create Folder" use a CheckBox instead of a CheckButton? Because the control does not have immediate effect (the folder is not immediately created).

I think a CheckButton works better because the button immediately changes the path field.

  • What the "Create Folder" button does was clear: Create a folder with the project name at the current project path, and use that folder as the new project path.

  • The "Create Folder" toggle is confusing:

    • When toggled off: It strips the last component from project path no matter if it's the same as project name.

      1. Turn on
      2. Set the project path to /path/to/something/you/typed/later
      3. Turn off
      4. And it strips /later
    • When toggled on: It appends the project name to the project path, even when the current path does not exist.

      1. Set the project path to /path/that/does/not/exist
      2. Turn on
      3. The project path becomes /path/that/does/not/exist/ProjectName
      4. It still reports error because the parent directory does not exist (and won't be created).
    • When "Create & Edit" is pressed: create the project folder if missing when the toggle is on.

I tried my best to make it intuitive and straightforward for most use-cases.

I've already considered changing the text on the button, but I couldn't decide on what to change it to.

Edit: I see two potential improvements, adding them to the root PR.

@nathanfranke nathanfranke changed the title Automatically create folder in project manager Automatically create folder when making a new project Jan 3, 2022
@AaronRecord
Copy link
Contributor

I agree that moving the button somewhere else would make sense, but the only other place I can think of would be below the name/path fields, taking more vertical space.

It could go on the right side of an hbox with the "Project Path:" label.

@nathanfranke nathanfranke marked this pull request as draft May 2, 2022 09:35
@nathanfranke nathanfranke changed the title Automatically create folder when making a new project Automatically create folder in project manager create and import Jun 26, 2022
@nathanfranke nathanfranke marked this pull request as ready for review June 26, 2022 01:59
@nathanfranke nathanfranke force-pushed the project-creation branch 2 times, most recently from c8df18a to 06a8330 Compare June 27, 2022 02:55
@nathanfranke
Copy link
Contributor Author

🎉
image

@Calinou
Copy link
Member

Calinou commented Jul 10, 2022

@nathanfranke Thanks for adding this 🙂

Does it prevent creating the project? It should, as users should never be able to brick their Godot editor installation entirely.

Also, I'd rephrase it slightly:

Creating a project in the engine's working directory or the home directory is not allowed, as it would prevent the editor from starting.

The check should be performed on both $CWD and $HOME regardless of the current path of the engine binary, in case you're starting the engine from $CWD but you intend to install it for use with $HOME later on.

@nathanfranke
Copy link
Contributor Author

Does it prevent creating the project?

Yes

How do I get the home directory?

@Calinou
Copy link
Member

Calinou commented Jul 10, 2022

How do I get the home directory?

Probably like this:

#ifdef WINDOWS_ENABLED
	const String home_dir = OS::get_singleton()->get_environment("USERPROFILE")
#else
	const String home_dir = OS::get_singleton()->get_environment("HOME")
#endif

	if (home_dir != "") {
		// Do check.
	}

OS.get_environment() returns an empty string if the environment variable doesn't exist, so make sure to only perform the check if the string is non-empty.

See also

const String fs_dir_default_project_path = OS::get_singleton()->has_environment("HOME") ? OS::get_singleton()->get_environment("HOME") : OS::get_singleton()->get_system_dir(OS::SYSTEM_DIR_DOCUMENTS);
.

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Sep 13, 2022

This is working great on my end and ready for review. Behavior is intuitive for me for every edge case, with the exception of one: Typing the project name as .. will put that same text .. in the path, which will almost definitely cause oddities. I traced the issue back to OS.get_safe_dir_name and made a PR to fix that behavior: #65755

To rephrase, this PR is complete, but PR #65755 should be merged first, and this PR retested, before this PR should be merged!

@akien-mga akien-mga modified the milestones: 4.0, 4.x Nov 24, 2022
@nathanfranke nathanfranke force-pushed the project-creation branch 3 times, most recently from ebcbedd to 1b60c84 Compare February 19, 2023 06:00
@nathanfranke
Copy link
Contributor Author

This is ready for 4.1 if desired

@nathanfranke nathanfranke marked this pull request as draft December 15, 2023 23:52
@YuriSizov
Copy link
Contributor

The code is somewhat difficult to review due to amount of changes, but I did some testing and overall it works correctly.

I second KoBeWi's concerns here, by the way. I have yet to look into this PR in detail, but why exactly the changes are so extensive for such an isolated feature? Can the changeset be reduced somehow?

Full disclosure, I need to work on the project manager, and this PR is going to get in the way. So I either have to accept it or ignore it. Making sure this PR doesn't exceed its own scope would be a great help.

@nathanfranke
Copy link
Contributor Author

I am reworking this to reduce the changes, will take a few days.

@YuriSizov
Copy link
Contributor

I am reworking this to reduce the changes, will take a few days.

Thanks, I appreciate it! I took a look over the weekend and I think it all pretty much makes sense and at least changes are isolated to one class, for the most part. Renames for clarity are also very nice to have. So at a glance I'm not sure what specifically can be simplified, but an attempt at that is very much welcome.

KoBeWi mentioned that perhaps the folder naming scheme feature can be cut, and I agree that it is probably not very necessary, but since it's already implemented then it makes sense to keep it.

@nathanfranke nathanfranke force-pushed the project-creation branch 3 times, most recently from 556e676 to f46bce7 Compare January 3, 2024 09:05
@akien-mga
Copy link
Member

This can be rebased / redone after #87266.

@@ -880,6 +880,9 @@
<member name="project_manager/default_renderer" type="String" setter="" getter="">
The renderer type that will be checked off by default when creating a new project. Accepted strings are "forward_plus", "mobile" or "gl_compatibility".
</member>
<member name="project_manager/directory_naming_convention" type="int" setter="" getter="">
Directory naming convention for the project manager. Options are "No convention" (project name is directory name), "kebab-case" (default), "snake_case", "camelCase", "PascalCase", or "Title Case".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "kebab-case" (default) the default for filenames? I thought it was snake_case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the default is kebab-case and it replaces underscores: t e_s-tt-e-s-t
It would probably be better to keep underscores instead: t e_s-tt-e_s-t

Could be implemented like this: dir_name = project_name.to_lower().replace(" ", "-")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default (kebab-case) is now implemented as to_lower().replace(" ", "-"). So underscores are preserved: t e_s-tt-e_s-t

@nathanfranke nathanfranke marked this pull request as ready for review February 27, 2024 09:46
@nathanfranke nathanfranke marked this pull request as draft February 27, 2024 09:48
@nathanfranke nathanfranke marked this pull request as ready for review March 8, 2024 08:15
@nathanfranke
Copy link
Contributor Author

Thanks for the patience, rebased and ready.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, looks pretty good! It's a great usability improvement, I really disliked how this worked until now.

The code changes are pretty hard to review in depth, for future PRs please try to keep refactoring separate from feature work. There's a lot of changes here which are just moving things around or slightly improving code quality which make the functional changes challenging to review. But overall it seems ok to me.

editor/project_manager/project_dialog.cpp Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.h Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.cpp Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.cpp Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.h Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.cpp Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.cpp Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.h Outdated Show resolved Hide resolved
editor/project_manager/project_dialog.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 8af66a7 into godotengine:master Mar 8, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented Mar 8, 2024

Thanks! And thanks for your patience and getting this PR to the finish line!

@allenwp
Copy link
Contributor

allenwp commented Jul 8, 2024

It seems that this PR introduced a usability issue described in #94081.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet